Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Clone requirement for argument types #353

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove Clone requirement for argument types #353

wants to merge 2 commits into from

Conversation

bnaecker
Copy link
Collaborator

  • Rework construction of the internal type-checking function to remove the clone requirement. Construct and call the type-check-function right at the site we already expand the probe argument closure, rather than outside. This means we don't need to expand things twice, and don't need to clone the closure, and so don't need the argument types to be cloneable either.
  • Fixup tests for new method, add some comments.
  • Update trybuild tests
  • Closes Remove unnecessary Clone requirement #136

@bnaecker bnaecker force-pushed the no-clone-closure branch 7 times, most recently from 9f35399 to 43b7c86 Compare December 18, 2024 23:35
@bnaecker
Remove Clone requirement for argument types
0477fd0 
- Rework construction of the internal type-checking function to remove
  the clone requirement. Construct and call the type-check-function
  right at the site we already expand the probe argument closure, rather
  than outside. This means we don't need to expand things twice, and
  don't need to clone the closure, and so don't need the argument types
  to be cloneable either.
- Fixup tests for new method, add some comments.
- Update trybuild tests
- Closes #136
@bnaecker bnaecker requested a review from ahl December 19, 2024 00:25
ahl
ahl approved these changes Dec 19, 2024
View reviewed changes
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good; you might try building crucible or something with this just to make sure nothing broke.

usdt-impl/src/no-linker.rs Outdated Show resolved Hide resolved
@bnaecker
Copy link
Collaborator Author

bnaecker commented Jan 18, 2025 •
edited
Loading

I pulled this branch into a binary with some probes in the Omicron repo, and it looks like the probes still fire as normal:

^Cbnaecker@flint : ~/file-cabinet/oxide/omicron/oximeter/db $ sudo dtrace -n 'clickhouse_io*:::' -p 1112
dtrace: description 'clickhouse_io*:::' matched 23 probes
CPU     ID                    FUNCTION:NAME
  3   4366 _ZN156_$LT$oximeter_db..native..io..packet..client..Encoder$u20$as$u20$tokio_util..codec..encoder..Encoder$LT$oximeter_db..nati:packet-sent
  3   4365 _ZN11oximeter_db6native10connection10Connection11query_inner28_$u7b$$u7b$closure$u7d$$u7d$17hef12990ca5dec621E:packet-sent
  0   4363 _ZN104_$LT$oximeter_db..native..io..packet..server..Decoder$u20$as$u20$tokio_util..codec..decoder..Decoder$GT$6decode17h485c565:packet-received
  0   4350 _ZN11oximeter_db6native10connection10Connection11query_inner28_$u7b$$u7b$closure$u7d$$u7d$17hef12990ca5dec621E:data-packet-received
  0   4363 _ZN104_$LT$oximeter_db..native..io..packet..server..Decoder$u20$as$u20$tokio_util..codec..decoder..Decoder$GT$6decode17h485c565:packet-received
  0   4350 _ZN11oximeter_db6native10connection10Connection11query_inner28_$u7b$$u7b$closure$u7d$$u7d$17hef12990ca5dec621E:data-packet-received
  0   4363 _ZN104_$LT$oximeter_db..native..io..packet..server..Decoder$u20$as$u20$tokio_util..codec..decoder..Decoder$GT$6decode17h485c565:packet-received
  0   4363 _ZN104_$LT$oximeter_db..native..io..packet..server..Decoder$u20$as$u20$tokio_util..codec..decoder..Decoder$GT$6decode17h485c565:packet-received
  3   4363 _ZN104_$LT$oximeter_db..native..io..packet..server..Decoder$u20$as$u20$tokio_util..codec..decoder..Decoder$GT$6decode17h485c565:packet-received
  3   4363 _ZN104_$LT$oximeter_db..native..io..packet..server..Decoder$u20$as$u20$tokio_util..codec..decoder..Decoder$GT$6decode17h485c565:packet-received
  3   4350 _ZN11oximeter_db6native10connection10Connection11query_inner28_$u7b$$u7b$closure$u7d$$u7d$17hef12990ca5dec621E:data-packet-received
  3   4363 _ZN104_$LT$oximeter_db..native..io..packet..server..Decoder$u20$as$u20$tokio_util..codec..decoder..Decoder$GT$6decode17h485c565:packet-received
  2   4363 _ZN104_$LT$oximeter_db..native..io..packet..server..Decoder$u20$as$u20$tokio_util..codec..decoder..Decoder$GT$6decode17h485c565:packet-received

That's while running some SQL queries against ClickHouse using a little shell I wrote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahl ahl ahl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove unnecessary Clone requirement
2 participants
@bnaecker @ahl